-
Notifications
You must be signed in to change notification settings - Fork 461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix uploads API when using NewBackends #733
Conversation
Geeze, sorry about the trouble here, and really excellent debugging work to get to the bottom of it. I think I'm going to try and unwind the Thanks! LGTM. |
Released as 54.1.1. |
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` prefix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one tha's been // passed for better backwards compatibility. config.URL = strings.TrimPrefix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` prefix. I've tried to address this by trimming a `/v1/` prefix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` suffix. I've tried to address this by trimming a `/v1/` suffix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
Traditionally, stripe-go has done something a little weird in that it considered the `/v1/` in API URLs as part of the URL (e.g., `APIURL` or `UploadURL`) instead of part of the paths of individual API calls. So when calling `POST /v1/charges` we'd call `POST /charges` and rely on the fact that `/v1/` was expected to be present in the URL. This has always rubbed me the wrong way a little bit, and caused a little bit of trouble in #733 as we'd been doing something a little hacky when initializing new configs for backends. This has some potential backwards compatibility problems in that people may have been injecting custom URLs (e.g., if they had a testing setup a little like we do for stripe-mock) that included the `/v1/` suffix. I've tried to address this by trimming a `/v1/` suffix when creating a backend with a configured URL. For example: ``` go switch backendType { case APIBackend: if config.URL == "" { config.URL = apiURL } // For a long time we had the `/v1` suffix as part of a configured URL // rather than in the per-package URLs throughout the library. Continue // to support this for the time being by stripping one that's been // passed for better backwards compatibility. config.URL = strings.TrimSuffix(config.URL, "/v1") config.URL = strings.TrimSuffix(config.URL, "/v1/") ``` I believe that will address the problem. There's some risk that I missed a URL string in the project because there are a lot of them. I tried to minimize the risk there by using a tool-assisted project-wide find + replace, then manually examining every string in the project that looks like a URL path (e.g., starts with `"/`).
When testing uploading files using an API client, I found that it would fail consistently with a strange error:
Adding some log statements to the
backendImplementation.Do
function, I found that it was trying to send afiles.New
request tohttps://api.stripe.com/v1/v1/files
Eventually I tracked the problem down to the
NewBackends
function. It reused the config object when creating the api and uploads backends. TheGetBackendWithConfig
function then modified the config object. So when it was called again with the same config object, it would already have the URL field filled out.Simply put, we need two copies of the config object to avoid causing problems with the modified config object.